Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

try to fix broken logo and favicon #126

Closed
wants to merge 2 commits into from
Closed

try to fix broken logo and favicon #126

wants to merge 2 commits into from

Conversation

pjfanning
Copy link
Contributor

see apache/pekko-site#95

needs more testing before merging

@pjfanning pjfanning marked this pull request as draft March 3, 2024 20:56
@pjfanning
Copy link
Contributor Author

pjfanning commented Mar 3, 2024 •
edited
Loading

@samueleresca @mdedetrich I built this locally and then used it in pekko-grpc paradox build (also locally) - but none of the assets.hostname dependent paths were mapped - all the links are relative. This includes the existing paths that already support assets.hostname.

edit - I tested with pekko-site repo and these changes seem to work there

@pjfanning pjfanning marked this pull request as ready for review March 3, 2024 21:36
@samueleresca
Copy link
Member

samueleresca commented Mar 3, 2024 •
edited
Loading

@samueleresca @mdedetrich I built this locally and then used it in pekko-grpc paradox build (also locally) - but none of the assets.hostname dependent paths were mapped - all the links are relative. This includes the existing paths that already support assets.hostname.

edit - I tested with pekko-site repo and these changes seem to work there

Have you tried to also specify the assets.hostname property into the downstream module (e.g.: paradoxProperties in incubator-pekko-(http|grpc))

I would expect the changes in this PR would fix the problem. The assets.hostname is specified at theme level and the logo element was not including the base path. The other bit that worries me is the $page.base$ part appended after the assets.hostname porperty, which might be the cause of the issue.

I can take a look at this tomorrow

@samueleresca samueleresca mentioned this pull request Mar 3, 2024
Closed
@pjfanning
Copy link
Contributor Author

pjfanning commented Mar 3, 2024 •
edited
Loading

The $page.properties.("assets.hostname")$$page.base$ pattern is already used in https://github.com/apache/incubator-pekko-sbt-paradox/blob/d47e88d12a922d678048306d9b069106162cb00f/theme/src/main/assets/page.st#L57

and other places in this file

@pjfanning pjfanning mentioned this pull request Mar 3, 2024
Merged
@mdedetrich
Copy link
Contributor

I can take a look at this tomorrow

Ill wait till tomorrow to see if there is an update on this

Roiocam
Roiocam reviewed Mar 4, 2024
View reviewed changes
theme/src/main/assets/page.st
@@ -46,7 +46,7 @@ $!
$ elseif (page.properties.("material.author")) $
<meta name="author" content="$page.properties.("site.author")$">
$ endif $
<link rel="shortcut icon" href="$page.base$$page.properties.("material.favicon")$">
<link rel="shortcut icon" href="$page.properties.("assets.hostname")$$page.base$$page.properties.("material.favicon")$">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt that this is related to #86. I'm not sure if page.properties ("assets.hostname") exists, but using it with $page.base$ may be a bad idea.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The icon link of all modules is broken, but the pekko homepage is not, which also proves this. $page.base$ is the path of a specific project.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link is to https://pekko.apache.org/docs/pekko-http/current/assets/images/pekko_logo.png

Should be https://pekko.apache.org/assets/images/pekko_logo.png

The previous comments should be no problem. $page.base$ is not a problem. At present, it is always the root directory of the project, that is.

The documents generated by all these modules (http, connector, pekko) are "assets/xxxx", even if the string template adds assets.home, but due to pekko, conn Ector These modules do not have the default value of assets.home, so they still do not point the path to CDN.

So I think the PR needs to set a default value for assets.home, or explicitly add the assets.home configuration value for each project.

@pjfanning
Copy link
Contributor Author

seems to be included in #127

@pjfanning pjfanning closed this Mar 4, 2024
@samueleresca samueleresca mentioned this pull request Mar 4, 2024
Merged
@samueleresca
Copy link
Member

Added an issue tracking the page.base paths in the static assets: #128

@pjfanning pjfanning deleted the broken-logo-favicon branch March 5, 2024 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Roiocam Roiocam Roiocam left review comments

@mdedetrich mdedetrich Awaiting requested review from mdedetrich

@samueleresca samueleresca Awaiting requested review from samueleresca

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pjfanning @samueleresca @mdedetrich @Roiocam